Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add a CONTRIBUTING.md #30

Merged
merged 7 commits into from
Dec 6, 2024
Merged

Add a CONTRIBUTING.md #30

merged 7 commits into from
Dec 6, 2024

Conversation

samcunliffe
Copy link
Member

based on UCL-ARC/python-tooling.

@paddyroddy strong opinion about the merging strategy?

@samcunliffe samcunliffe linked an issue Dec 2, 2024 that may be closed by this pull request
Add a CONTRIBUTING.md #24
Closed
@samcunliffe samcunliffe self-assigned this Dec 2, 2024
samcunliffe
samcunliffe commented Dec 2, 2024
View reviewed changes
CONTRIBUTING.md Outdated Show resolved Hide resolved
@samcunliffe samcunliffe mentioned this pull request Dec 2, 2024
Merged
dstansby pushed a commit to UCL-ARC/python-tooling that referenced this pull request Dec 2, 2024
@samcunliffe
Fix NOTE in README (#500)
90ff199 
Probably the most exciting PR you've seen (in your lives?).

Related to fun I was having over in
- UCL-ARC/coding-standards#30
milanmlft
milanmlft approved these changes Dec 4, 2024
View reviewed changes
Copy link
Member

@milanmlft milanmlft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀
For the merge strategy, I'd enforce that through the repo settings, don't think that needs to go in the CONTIBUTING.md necessarily.

CONTRIBUTING.md Outdated Show resolved Hide resolved
@samcunliffe @milanmlft
I can English such goodly.
6b0cf7e 
Co-authored-by: Milan Malfait <38256462+milanmlft@users.noreply.github.com>
@paddyroddy
Copy link
Member

🚀 For the merge strategy, I'd enforce that through the repo settings, don't think that needs to go in the CONTIBUTING.md necessarily.

I agree with this

@samcunliffe
Copy link
Member Author

🚀 For the merge strategy, I'd enforce that through the repo settings, don't think that needs to go in the CONTIBUTING.md necessarily.

I agree with this

My question was rather:

5. If approved with no comments, then the last approving reviewer should merge the PR.
6. If changes are requested, the PR author should address the comments, and then ask for review again.

What do you want me to do here. Shall I change it? Shall we have a rule that auto merges upon approval of one if no comments?

paddyroddy
paddyroddy approved these changes Dec 6, 2024
View reviewed changes
Copy link
Member

@paddyroddy paddyroddy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

(squashing FTW - enforced in settings)

CONTRIBUTING.md
Comment on lines +12 to +13
<!-- prettier-ignore -->
> [!NOTE]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will mean only the [!NOTE] is not formatted. The rest will be (which is the bit that isn't broken).

CONTRIBUTING.md
Comment on lines +30 to +31
[GitHub flavoured markdown]:
https://docs.github.com/en/get-started/writing-on-github/getting-started-with-writing-and-formatting-on-github/basic-writing-and-formatting-syntax
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is valid syntax, so no need to ignore prettier here either

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@paddyroddy
Copy link
Member

What do you want me to do here. Shall I change it? Shall we have a rule that auto merges upon approval of one if no comments?

Automerging should be used with great caution

@samcunliffe samcunliffe merged commit 8a88d7b into main Dec 6, 2024
1 check passed
@samcunliffe samcunliffe deleted the add-contributing-dot-md branch December 6, 2024 12:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@milanmlft milanmlft milanmlft approved these changes

@paddyroddy paddyroddy paddyroddy approved these changes

@HChughtai HChughtai Awaiting requested review from HChughtai

@K-Meech K-Meech Awaiting requested review from K-Meech

@pineapple-cat pineapple-cat Awaiting requested review from pineapple-cat

Assignees

@samcunliffe samcunliffe

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add a CONTRIBUTING.md
3 participants
@samcunliffe @paddyroddy @milanmlft